Use `MonitorUpdatingPersister` by arturgontijo · Pull Request #456 · lightningdevkit/ldk-node

arturgontijo · 2025-02-01T16:02:17Z

This PR aims to solve #441 , making use of MonitorUpdatingPersister in the ChainMonitor.

Note: Not sure about a proper value for its maximum_pending_updates (set it to 10 for now).

enigbe · 2025-02-10T23:58:29Z

src/io/test_utils.rs

+	let persister_0 = MonitorUpdatingPersister::new(
+		store_0,
+		&chanmon_cfgs[0].logger,
+		persister_0_max_pending_updates,
+		&chanmon_cfgs[0].keys_manager,
+		&chanmon_cfgs[0].keys_manager,
+		&chanmon_cfgs[0].tx_broadcaster,
+		&chanmon_cfgs[0].fee_estimator,
+	);
+
+	let persister_1 = MonitorUpdatingPersister::new(
+		store_1,
+		&chanmon_cfgs[1].logger,
+		persister_1_max_pending_updates,
+		&chanmon_cfgs[1].keys_manager,
+		&chanmon_cfgs[1].keys_manager,
+		&chanmon_cfgs[1].tx_broadcaster,
+		&chanmon_cfgs[1].fee_estimator,
+	);


Given the duplication, consider extracting this into a helper function.

enigbe · 2025-02-11T00:02:37Z

src/builder.rs

+	let persister = Arc::new(Persister::new(
+		Arc::clone(&kv_store),
+		Arc::clone(&logger),
+		10, // (?)


I also don't know what value to recommend here, but pending when @tnull takes a look, you could define a constant to hold this value.

My guess is as good as any, as we don't have super good data on this parameter so far. I think we could bump it to 100 to start with and then see if we run into any issue with it. I agree that we should introduce a const for it though.

FWIW I agree with you @tnull.

Our node ran on 100 for a long time and is 1000 now, but you would probably want 100 for most people. I dunno your design philosophy about what to make configurable, but it's probably a small number of power users who would want something other than 100.

enigbe

Hi @arturgontijo, thanks you for submitting this PR. I have done an initial review and left two minor comments regarding the hard-coding of values and using helper functions to reduce duplicated logic. From my POV, this looks good and I am happy to take another look when the comments are addressed.

tnull

Cool, thank you very much for looking into this! Excuse the delay, I have to admit it fell of my radar for a few days.

The changes already look pretty good to me, I just have a few minor nits / comments.

Given that this patch, while rather small, affects really the core of the node's functionality, I'm considering holding off landing this for a few days until after the upcoming 0.5 release. This would give us more time to spot any funky behavior that could arise from this, especially since there have been considerable changes made to the MonitorUpdatingPersister on LDK main recently that will ship with LDK v0.2, and further changes to the persistence/storage layer in LDK Node are planned for v0.6.

TLDR: while the patch is pretty simple, I'll go ahead and tag this PR for milestone 0.6, hopefully landing soon after v0.5 shipped.

tnull · 2025-02-11T08:05:07Z

src/builder.rs

+	let persister = Arc::new(Persister::new(
+		Arc::clone(&kv_store),
+		Arc::clone(&logger),
+		10, // (?)


My guess is as good as any, as we don't have super good data on this parameter so far. I think we could bump it to 100 to start with and then see if we run into any issue with it. I agree that we should introduce a const for it though.

tnull · 2025-02-11T08:06:51Z

src/io/test_utils.rs


 use lightning::events::ClosureReason;
 use lightning::util::test_utils;
-use lightning::{check_added_monitors, check_closed_broadcast, check_closed_event};


nit: Not quite sure why the import was changed, but if we already do so, why not move the others up, too?

tnull · 2025-02-11T08:11:39Z

src/io/test_utils.rs

+
+				let monitor_name = MonitorName::from(mon.get_funding_txo().0);
+				assert_eq!(
+					store_0


nit: Would prefer if we could move this to a variable to avoid the additional indentation and verticality in the assert_eq (same below).

tnull · 2025-02-11T08:14:39Z

src/io/test_utils.rs

+			sender = 0;
+			receiver = 1;
+		}
+		send_payment(&nodes[sender], &vec![&nodes[receiver]][..], 21_000);


You can avoid the unnecessary allocations here and below by simply dropping vec!, given that you're making it a slice anyways.

Suggested change

send_payment(&nodes[sender], &vec![&nodes[receiver]][..], 21_000);

send_payment(&nodes[sender], &[&nodes[receiver]][..], 21_000);

also, consider moving these to appropriately named variables for clarity.

arturgontijo · 2025-02-11T11:09:41Z

Hey @enigbe and @tnull, thanks for the initial review.

Regarding test_utils.rs, I kept it as a copy&paste but now from rust-lightning/.../persist.rs.
But I totally agree that, as it is part of this repo now, we should make it better =)

I'll address all the suggestions and also agree that it can bring side effects that we are not anticipating yet.

G8XSU · 2025-03-11T21:30:51Z

Given that this patch, while rather small, affects the core of the node's functionality, I'm considering holding off on landing it for a few days until after the upcoming 0.5 release.

@tnull, I would prefer if we prioritize this over the big refactor in 0.6 and ldk-0.2. This is mainly because it would be best if ldk-server supported MonitorUpdatingPersister in 0.1 itself. Since the changes are simple enough and we expect any other v2-channel related changes to be backward compatible, we should be able to prioritize this, right?

However, one drawback of the current approach in the PR is that "LDK Node moves to MonitorUpdatingPersister" without user request/input. We might want to consider that bit carefully or at least make it optional. Since, once the node's persistence moves to MonitorUpdatingPersister, it isn’t straightforward to revert to not using MonitorUpdatingPersister.

tnull · 2025-03-13T09:41:34Z

@tnull, I would prefer if we prioritize this over the big refactor in 0.6 and ldk-0.2. This is mainly because it would be best if ldk-server supported MonitorUpdatingPersister in 0.1 itself. Since the changes are simple enough and we expect any other v2-channel related changes to be backward compatible, we should be able to prioritize this, right?

Well, yes, I intend to land this ~first thing after 0.5 shipped. So it will be prioritized for 0.6, but we will (need to) add the refactors in that release.

However, one drawback of the current approach in the PR is that "LDK Node moves to MonitorUpdatingPersister" without user request/input. We might want to consider that bit carefully or at least make it optional. Since, once the node's persistence moves to MonitorUpdatingPersister, it isn’t straightforward to revert to not using MonitorUpdatingPersister.

That's a good point and btw one of the reason why I decided to hold off on this PR for 0.5: I first want to take some time to think through (and test) some of the consequences, especially to figure out if there are relevant use cases that would prefer the old style, and if we should support them.

tnull · 2025-04-29T09:03:33Z

@arturgontijo We now shipped 0.5, so could consider to move forward with this PR soon. It also needs a rebase by now.

arturgontijo · 2025-04-30T11:59:41Z

@arturgontijo We now shipped 0.5, so could consider to move forward with this PR soon. It also needs a rebase by now.

Cool, let me resolve that conflict and then we can discuss next steps.

# Conflicts: # src/io/test_utils.rs

amackillop · 2025-06-16T19:48:41Z

However, one drawback of the current approach in the PR is that "LDK Node moves to MonitorUpdatingPersister" without user request/input. We might want to consider that bit carefully or at least make it optional. Since, once the node's persistence moves to MonitorUpdatingPersister, it isn’t straightforward to revert to not using MonitorUpdatingPersister.

I can give some input as a user. After about 20k channel monitor updates payments start to slow down substantially. This issue was noticed a few years ago on our routing node and using this MonitorUpdatingPersister fixed the issue

tnull · 2025-06-18T11:13:09Z

However, one drawback of the current approach in the PR is that "LDK Node moves to MonitorUpdatingPersister" without user request/input. We might want to consider that bit carefully or at least make it optional. Since, once the node's persistence moves to MonitorUpdatingPersister, it isn’t straightforward to revert to not using MonitorUpdatingPersister.

I can give some input as a user. After about 20k channel monitor updates payments start to slow down substantially. This issue was noticed a few years ago on our routing node and using this MonitorUpdatingPersister fixed the issue

Thanks for the feedback! Btw, you might be interested in the related work going over at rust-lightning: lightningdevkit/rust-lightning#3834 (cc @domZippilli)

domZippilli · 2025-09-29T14:54:16Z

Why was this closed?

arturgontijo · 2025-09-29T16:24:19Z

Why was this closed?

This PR got blocked for some reason (that I can't recall now) and I decided to move on to another stuff, feel free to pick things here and open a new PR

arturgontijo added 2 commits February 1, 2025 12:25

Use MonitorUpdatingPersister

14494e1

fmt

907677f

enigbe reviewed Feb 10, 2025

View reviewed changes

enigbe reviewed Feb 11, 2025

View reviewed changes

tnull reviewed Feb 11, 2025

View reviewed changes

tnull added this to the 0.6 milestone Feb 11, 2025

tnull linked an issue Feb 11, 2025 that may be closed by this pull request

Switch to MonitorUpdatingPersister #441

Closed

arturgontijo added 2 commits February 18, 2025 20:48

Merge branch 'refs/heads/main' into monitor-upd-persister

95e010b

Code review

5a65e04

arturgontijo changed the title ~~Use MonitorUpdatingPersister~~ Use MonitorUpdatingPersister Feb 19, 2025

arturgontijo added 2 commits May 5, 2025 10:00

Merge branch 'refs/heads/main' into monitor-upd-persister

b8eb376

# Conflicts: # src/io/test_utils.rs

sync master + fmt

9bb5a6e

tnull modified the milestones: 0.6, 0.7 Jun 9, 2025

arturgontijo closed this Jul 23, 2025

tnull mentioned this pull request Sep 12, 2025

Add support for native async KVStore persist to ChainMonitor lightningdevkit/rust-lightning#4063

Merged

tnull mentioned this pull request Oct 13, 2025

Use MonitorUpdatingPersister #661

Merged

	send_payment(&nodes[sender], &vec![&nodes[receiver]][..], 21_000);
	send_payment(&nodes[sender], &[&nodes[receiver]][..], 21_000);

Conversation

arturgontijo commented Feb 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enigbe left a comment

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgontijo commented Feb 11, 2025

Uh oh!

G8XSU commented Mar 11, 2025

Uh oh!

tnull commented Mar 13, 2025

Uh oh!

tnull commented Apr 29, 2025

Uh oh!

arturgontijo commented Apr 30, 2025

Uh oh!

amackillop commented Jun 16, 2025

Uh oh!

tnull commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domZippilli commented Sep 29, 2025

Uh oh!

arturgontijo commented Sep 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tnull commented Jun 18, 2025 •

edited

Loading